Skip to content

Fix: Use gas limit estimate by default#787

Open
renatomaia wants to merge 3 commits into
next/2.0from
fix/UseGasLimitEstimateByDefault
Open

Fix: Use gas limit estimate by default#787
renatomaia wants to merge 3 commits into
next/2.0from
fix/UseGasLimitEstimateByDefault

Conversation

@renatomaia

Copy link
Copy Markdown

This PR changes CLI transaction submission to use the gas limit estimated by the Ethereum client by default instead of forcing a fixed 30_000_000 gas limit.

It also adds CARTESI_BLOCKCHAIN_GAS_LIMIT as an optional configuration value for deployments that still need to override the gas limit manually. The default is 0, which leaves bind.TransactOpts.GasLimit unset and allows go-ethereum contract bindings to estimate gas.

Changes

  • Remove the hardcoded ethutil.GasLimit from CLI transaction paths.

  • Add CARTESI_BLOCKCHAIN_GAS_LIMIT config support across node, claimer, evmreader, and prt config.

  • Apply a non-zero configured gas limit in auth.GetTransactOpts.

  • Add a hidden --gas-limit CLI flag bound to CARTESI_BLOCKCHAIN_GAS_LIMIT.

  • Preserve the original transaction error cause when self-hosted application deployment fails.

  • Add focused tests for gas limit propagation in auth.GetTransactOpts.

    Testing

  • Added coverage for:

    • default gas limit 0 leaving txOpts.GasLimit unset
    • non-zero CARTESI_BLOCKCHAIN_GAS_LIMIT setting txOpts.GasLimit

Fixes: #786

@renatomaia renatomaia requested review from mpolitzer and vfusco June 29, 2026 12:47
mpolitzer
mpolitzer previously approved these changes Jun 30, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates transaction submission across the CLI/ethutil paths to stop forcing a fixed 30M gas limit and instead default to go-ethereum gas estimation (GasLimit = 0), while introducing an optional CARTESI_BLOCKCHAIN_GAS_LIMIT override that can be set via config/env or a hidden --gas-limit flag.

Changes:

  • Removed the hardcoded gas limit plumbing (GasLimit = 30_000_000) so contract bindings can estimate gas by default.
  • Added CARTESI_BLOCKCHAIN_GAS_LIMIT to the generated configuration surface and applied it in auth.GetTransactOpts when non-zero.
  • Added tests ensuring gas limit propagation in auth.GetTransactOpts, and adjusted self-hosted deployment error wrapping.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/ethutil/transaction.go Stops forcing a gas limit into TransactOpts, enabling default estimation.
pkg/ethutil/selfhosted.go Updates deployment transaction call path; adjusts error wrapping on failure.
pkg/ethutil/ethutil.go Removes usage of the deleted fixed gas limit constant across CLI write paths.
internal/config/generated.go Adds CARTESI_BLOCKCHAIN_GAS_LIMIT constant/default, structs, loaders, and getter.
internal/config/generate/Config.toml Declares the new CARTESI_BLOCKCHAIN_GAS_LIMIT config entry for generation.
internal/config/auth/auth.go Applies the configured gas limit override to returned TransactOpts.
internal/config/auth/auth_test.go Adds coverage for default (0) vs configured gas limit behavior.
cmd/cartesi-rollups-cli/root/root.go Adds hidden --gas-limit flag bound to CARTESI_BLOCKCHAIN_GAS_LIMIT.
Files not reviewed (1)
  • internal/config/generated.go: Generated file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/config/generated.go
Comment thread pkg/ethutil/selfhosted.go
@renatomaia renatomaia force-pushed the fix/UseGasLimitEstimateByDefault branch from ce20221 to a976a93 Compare July 2, 2026 17:16
@renatomaia renatomaia force-pushed the fix/UseGasLimitEstimateByDefault branch from a976a93 to bfbc92b Compare July 2, 2026 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants